Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

[2.1] Form File Upload refactor #2439

Closed
wants to merge 16 commits into from

Conversation

cgmartin
Copy link
Contributor

This PR refactors File Uploads to use Zend\Form's normal input filtering rather then using Zend\File\Transfer.
Ready to merge

Notes:

  • Zend\File\Transfer and its HTTP adapter is no longer used.
  • Filters and Validators are added to File's input filter, just like any other form input.
  • New Zend\InputFilter\FileInput will do validation first, then filtering.
  • InputFilter has been modified to detect if File Upload is missing, for isRequired/isEmpty tests.
  • New Zend\Filter\File\RenameUpload for securely moving uploaded files.
  • Refactored Zend\File\Transfer\Adapter\Http upload progress methods into Zend\ProgressBar\Upload handlers: Apc, UploadProgress, and (new) PHP5.4+ Session Upload Progress.
  • New Zend\Form\Element\File elements for Apc, UploadProgress, and Session upload progress tracking.
  • Multi-file uploads work successfully with Zend\Form\Element\Collection and when the HTML5 "multiple" attribute is set.
  • New Zend\Validator\File\Explode validator for "multiple" attribute validation.

Important to note: You must be cognizant to pass the $_FILES info into the Form:

$data = array_merge(
    $this->getRequest()->getPost()->toArray(),
    $this->getRequest()->getFiles()->toArray()
);
$form->setData($data);

Full usage example here:
https://gist.github.com/3797983

$filter = new InputFilter();
$filter->add($this->object->getInputFilter(), $name);
$this->filter = $filter;
if (null === $this->filter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bugfix: dupe filters being added on multiple calls to getInputFilter().

@bakura10
Copy link
Contributor

Really good work ! This was really missing. I've quickly read the source, it seems good to me.

I wonder about one thing : when hydrated, what will the element contain ? the file name ?

@cgmartin
Copy link
Contributor Author

@bakura10 When hydrated, the element's value "should" contain a string of the file path. I need to test this though - thanks for bringing it up.

@weierophinney
Copy link
Member

I really like this new approach. Having the FileInput as part of InputFilter makes a lot of sense to me, and I appreciate how much it simplifies the validators.

Looks like you may need to rebase off of develop, btw.

Keep me posted, and let me know when it's ready to review for merge!

@cgmartin
Copy link
Contributor Author

cgmartin commented Oct 2, 2012

Thanks @weierophinney. I'll pick this back up and ping you when it's ready for a final review.

@cgmartin
Copy link
Contributor Author

cgmartin commented Oct 3, 2012

Will this UploadFilter have the capability handle a succesfull upload, but other elements failed to validate use cases? Which would give the ability to just repost the (corrected) form without the file, but still have access to it?

I've been thinking about this too, but haven't come up with a generic/configurable/automatic way to do this with the form yet. There seemed to be several use cases one may want to do in this situation, so in an effort to get this out and tested, we'll leave it up to the developer to do their own custom handling for now.

File validations and filters will still run and process the file. But, it will be up to the developer to explicitly check that the file element is valid after postback and then modify the form/view to compensate, update the db, etc.. In some cases you might be able to separate out the file uploads into their own form to get around this, but not always.

@bakura10
Copy link
Contributor

bakura10 commented Oct 3, 2012

What about using event ?

Envoyé de mon iPhone

Le 3 oct. 2012 à 16:14, Chris Martin notifications@github.com a écrit :

Will this UploadFilter have the capability handle a succesfull upload, but other elements failed to validate use cases? Which would give the ability to just repost the (corrected) form without the file, but still have access to it?

I've been thinking about this too, but haven't come up with a generic/configurable/automatic way to do this with the form yet. There seemed to be several use cases one may want to do in this situation, so in an effort to get this out and tested, we'll leave it up to the developer to do their own custom handling for now.

File validations and filters will still run and process the file. But, it will be up to the developer to explicitly check that the file element is valid after postback and then modify the form/view to compensate, update the db, etc.. In some cases you might be able to separate out the file uploads into their own form to get around this, but not always.


Reply to this email directly or view it on GitHub.

@cgmartin
Copy link
Contributor Author

cgmartin commented Oct 6, 2012

This is ready for a final review. Thanks!

@juriansluiman
Copy link
Contributor

It will be fantastic to have this merged for 2.1! I almost don't dare to ask, but what about docs? It would be nice when this gets merged, the docs can be merged directly too.

@bakura10
Copy link
Contributor

bakura10 commented Oct 6, 2012

Wow... you need such an awesome work @cgmartin ! That's outstanding ! I can't wait to try this out !

@cgmartin
Copy link
Contributor Author

cgmartin commented Oct 6, 2012

@juriansluiman yes, already planning to work on the docs after this gets a final approval. I wouldn't leave you hanging like that! ;) Currently working on usage examples to see if this could fit with the postRequestGet plugin somehow.

@bakura10 thanks!

@basz
Copy link
Contributor

basz commented Oct 6, 2012

First +1 for you! Thanks

Op 6 okt. 2012 om 16:15 heeft Chris Martin notifications@github.com het volgende geschreven:

@juriansluiman yes, already planning to work on the docs after this gets a final approval. I wouldn't leave you hanging like that! ;)

Currently working on usage examples to see if this could fit with the postRequestGet plugin somehow.

That would be a really nice one as it could be used to also handle validation errors on other input while not having to upload the file again - or am I hoping for too much in this release..?
@bakura10 thanks!


Reply to this email directly or view it on GitHub.

@cgmartin
Copy link
Contributor Author

cgmartin commented Oct 6, 2012

Found an issue when using the HTML5 "multiple" attribute with the file element. Working on a fix. Do not merge.

@cgmartin
Copy link
Contributor Author

cgmartin commented Oct 6, 2012

OK this should be good for a final review again.

A "nice-to-have" would be an Explode Filter, akin to the Explode validators when the multiple attribute is set. IMO, I don't think that should hold up this PR, but I'll work on getting it in regardless.

@basz yep, working on it. We'll see how it goes.

@davidwindell
Copy link
Contributor

A few questions as we are currently using the existing implementation, will we still be able to achieve all the functionality as shown in the following gist? https://gist.github.com/83f582c74d45174e0538, for example;

  • Specifying the temporary destination
  • Getting the File Info data on the uploaded files for mimetype logging

Anything else you can see in there that might not work?

@weierophinney
Copy link
Member

@cgmartin Can you rebase against latest develop? There are evidently conflicts currently, and I don't want to mess up the merge.

Thanks!

@cgmartin
Copy link
Contributor Author

@davidwindell you'll still be able to achieve those things, though the implementation will be different, obviously. The FileSize, Extension, MimeType validators still exist; Count would now be managed by a Collection; and the destination can be specified via the RenameUpload filter. Getting the mime info for your attachment looks to be the only difference - You would likely use FileInfo (or similar) directly yourself for that, as the form processing will not save that information automatically. I haven't looked closely at it, but Zend\Mime may provide some automatic features for mail attachments.

@weierophinney, rebased. Thanks!

Now that ZendCon is over I'll be picking back up on the docs and the following enhancements in the following week:

Post-Redirect-Get plugin support
Discussed briefly with @weierophinney that we could potentially process the form's file elements (running validation and filtering) during the post, and save the error messages (and other state) for after the redirect. The form file elements must process on POST for is_uploaded_file security validation.

Multi-File Filtering
The Form will require a "Explode" filter (similar to the Explode validator) in order to process multiple files.

@cgmartin
Copy link
Contributor Author

cgmartin commented Nov 4, 2012

Travis failed only on 5.3.3 tests due to network connectivity, see details. Latest commit should pass.

Added a new Post-Redirect-Get plugin for File upload forms:
https://github.com/zendframework/zf2/pull/2439/files#L21R25

It is fundamentally different than the original plugin as it will process the form on POST and save any form errors in the session when it comes back around on the GET. Usage is different, see example here: https://github.com/cgmartin/ZF2FileUploadExamples/blob/master/src/ZF2FileUploadExamples/Controller/Examples.php#L75

@weierophinney
Copy link
Member

STELLAR work, @cgmartin !!!!

@bakura10
Copy link
Contributor

bakura10 commented Nov 6, 2012

Can't wait to use that ! Thank you @cgmartin !

@juriansluiman
Copy link
Contributor

👍 👏 🤘 whohoo!

@cgmartin
Copy link
Contributor Author

cgmartin commented Nov 6, 2012

Thanks, @weierophinney. It's neat that it was able to fit into your Form/InputFilter design. Designing Beautiful Software, indeed! ;)

Still working on the examples and docs. And I'll make a separate PR for the Explode filter.

On Nov 6, 2012, at 3:11 PM, weierophinney notifications@github.com wrote:

STELLAR work, @cgmartin !!!!


Reply to this email directly or view it on GitHub.

@mbn18
Copy link
Contributor

mbn18 commented Mar 13, 2013

@cgmartin , is it possible to create an InputFilter for file upload using InputFactory?

I tried different variant like this. but all failed.

$factory = new InputFactory();

$this->add($factory->createInputFilter(array(
    'name'          => 'file',
    'required'      => true,
)));

@cgmartin
Copy link
Contributor Author

@mbn18
Copy link
Contributor

mbn18 commented Mar 14, 2013

@cgmartin , tried that with no success.

I think that type is not the problem as it is already being declared at:
https://github.com/zendframework/zf2/blob/master/library/Zend/InputFilter/Factory.php#L217

I tried that but got an error.

        $this->add($factory->createInputFilter(array(
            'name'          => 'file',
            'required'      => true,
            'type'          => 'Zend\InputFilter\FileInput',
        )));
File:
/srv/httpd/brainpop/dev/web/vendor/ZendFramework/library/Zend/InputFilter/Factory.php:231
Message:
InputFilter factory expects the "type" to be a class implementing Zend\InputFilter\InputFilterInterface; received "Zend\InputFilter\FileInput"

Maybe the class is not defined yet or not compatible?

@cgmartin
Copy link
Contributor Author

Ah, we both may have confused by the differences between InputFilter vs. Input.

Assuming $this is your form's InputFilter, try this:

        $this->add($factory->createInput(array(
            'name'          => 'file',
            'required'      => true,
            'type'          => 'Zend\InputFilter\FileInput',
        )));

The InputFilter acts as a parent collection of Input objects (and potentially other InputFilters). The Input objects themselves contain the required, filters, validators details for each form input.
http://framework.zend.com/manual/2.1/en/modules/zend.input-filter.intro.html

The File Element utilizes $factory->createInput() rather than $factory->createInputFilter(). Here is the correct place where that type is checked in the factory:
https://github.com/zendframework/zf2/blob/master/library/Zend/InputFilter/Factory.php#L109

The file elements need a special FileInput in order to do the validations and filters correctly for file uploads. Some details here about the differences:
http://framework.zend.com/manual/2.1/en/modules/zend.input-filter.file-input.html

Hope this helps, let us know if still encountering a problem.

@mbn18
Copy link
Contributor

mbn18 commented Mar 15, 2013

Hi @cgmartin ,

Here is the full version ( without other fields )

<?php
namespace WhatEver\Form;

use Zend\InputFilter\InputFilter;
use Zend\InputFilter\Factory as InputFactory;

class AmazingWonderfulFilter extends InputFilter
{
    public function __construct()
    {
        $factory = new InputFactory();

        $this->add($factory->createInput(array(
            'name'          => 'file',
            'required'      => true,
            'type'          => 'Zend\InputFilter\FileInput',
        )));
    } // End of __construct
}

And the output ( I get the same error with and without declaring type )

An error occurred
An error occurred during execution; please try again later.
Additional information:
Zend\View\Exception\InvalidArgumentException
File:
/srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/Helper/Escaper/AbstractHelper.php:99
Message:
Array provided to Escape helper, but flags do not allow recursion
Stack trace:
#0 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Form/View/Helper/AbstractHelper.php(228): Zend\View\Helper\Escaper\AbstractHelper->__invoke(Array)
#1 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Form/View/Helper/FormInput.php(111): Zend\Form\View\Helper\AbstractHelper->createAttributesString(Array)
#2 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Form/View/Helper/FormInput.php(130): Zend\Form\View\Helper\FormInput->render(Object(Zend\Form\Element\File))
#3 [internal function]: Zend\Form\View\Helper\FormInput->__invoke(Object(Zend\Form\Element\File))
#4 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/Renderer/PhpRenderer.php(400): call_user_func_array(Object(Zend\Form\View\Helper\FormInput), Array)
#5 /srv/httpd/myproj/dev/web/module/ProjActivity/view/activity/view_revision.phtml(99): Zend\View\Renderer\PhpRenderer->__call('formInput', Array)
#6 /srv/httpd/myproj/dev/web/module/ProjActivity/view/activity/view_revision.phtml(99): Zend\View\Renderer\PhpRenderer->formInput(Object(Zend\Form\Element\File))
#7 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/Renderer/PhpRenderer.php(507): include('/srv/httpd/brai...')
#8 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/View.php(201): Zend\View\Renderer\PhpRenderer->render(Object(Zend\View\Model\ViewModel))
#9 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/View.php(229): Zend\View\View->render(Object(Zend\View\Model\ViewModel))
#10 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/View.php(194): Zend\View\View->renderChildren(Object(Zend\View\Model\ViewModel))
#11 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Mvc/View/Http/DefaultRenderingStrategy.php(126): Zend\View\View->render(Object(Zend\View\Model\ViewModel))
#12 [internal function]: Zend\Mvc\View\Http\DefaultRenderingStrategy->render(Object(Zend\Mvc\MvcEvent))
#13 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/EventManager/EventManager.php(460): call_user_func(Array, Object(Zend\Mvc\MvcEvent))
#14 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/EventManager/EventManager.php(204): Zend\EventManager\EventManager->triggerListeners('render', Object(Zend\Mvc\MvcEvent), Array)
#15 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Mvc/Application.php(332): Zend\EventManager\EventManager->trigger('render', Object(Zend\Mvc\MvcEvent))
#16 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Mvc/Application.php(307): Zend\Mvc\Application->completeRequest(Object(Zend\Mvc\MvcEvent))
#17 /srv/httpd/myproj/dev/web/public/index.php(12): Zend\Mvc\Application->run()
#18 {main}

@davidwindell
Copy link
Contributor

@mbn18 what does your view script look like?

@mbn18
Copy link
Contributor

mbn18 commented Mar 15, 2013

The view script is quite big. The relevant lines are:

<?php echo $this->formLabel($update_activity_rev_form->get('file')); ?>
<?php echo $this->formInput($update_activity_rev_form->get('file')->setAttributes(array('style'=>'margin-left: 10px;'))); ?>

But the error occur when I run an update controller (no view)

The form spec is:

<?php
namespace WhatEver\Form;

use Zend\Form\Factory as FormFactory;
use Zend\Form\Form;

class UpdateFooForm extends Form
{
    public function __construct()
    {
        parent::__construct($name);
        $factory = new FormFactory();

        $this->add($factory->createElement(array(
            'name' => 'file',
            'type' => 'Zend\Form\Element\File',
            'options' => array(
                'label' => 'Upload:',
            ),
        )));
    }
}

@weierophinney
Copy link
Member

Why aren't you using the formFile() helper?

On Friday, March 15, 2013, mbn18 wrote:

The view script is quite big. The relevant lines are:

formLabel($update_activity_rev_form->get('file')); ?>formInput($update_activity_rev_form->get('file')->setAttributes(array('style'=>'margin-left: 10px;'))); ?>

But the error occur when I run an update controller (no view)

The form spec is:

add($factory->createElement(array( 'name' => 'file', 'type' => 'Zend\Form\Element\File', 'options' => array( 'label' => 'Upload:', ), ))); }} ``` — Reply to this email directly or view it on GitHubhttps://github.com//pull/2439#issuecomment-14949923 .

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

@juriansluiman
Copy link
Contributor

/me thanks @weierophinney for suggesting a view helper I did not know about :-)

@davidwindell
Copy link
Contributor

@mbn18 @weierophinney Yep, that's why I wanted to see your view, just swap out the formInput with formFile or even formRow and you're good.

@mbn18
Copy link
Contributor

mbn18 commented Mar 21, 2013

Yep, that was the problem. Thanks everyone.
Maybe it will be beneficial to add it to the docs? If so, I can do it. Where it is done?

@cgmartin
Copy link
Contributor Author

The docs I created back in the 2.1 release have some details:
http://framework.zend.com/manual/2.1/en/modules/zend.form.view.helpers.html#formfile
http://framework.zend.com/manual/2.1/en/modules/zend.form.file-upload.html

Feel free to open a PR against the ZF2 Docs repo if they could use any enhancements:
https://github.com/zendframework/zf2-documentation

weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-progressbar that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-file that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants